-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fmt: implement Default for FmtOptions #4747
fmt: implement Default for FmtOptions #4747
Conversation
f711d6d
to
7644a75
Compare
7644a75
to
167ac6e
Compare
In the latest push I adapted the defaults to match those of GNU fmt. |
GNU testsuite comparison:
|
src/uu/fmt/src/fmt.rs
Outdated
@@ -140,7 +148,7 @@ fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions) | |||
), | |||
)); | |||
} | |||
fmt_opts.goal = cmp::min(fmt_opts.width * 94 / 100, fmt_opts.width - 3); | |||
fmt_opts.goal = cmp::min(fmt_opts.width * 93 / 100, fmt_opts.width - 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to move 93 into a const with a good variable name?
to make it more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope it is more clear now.
167ac6e
to
ec44577
Compare
GNU testsuite comparison:
|
ec44577
to
21d30a9
Compare
GNU testsuite comparison:
|
@@ -155,7 +169,10 @@ fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions) | |||
} | |||
}; | |||
if !matches.get_flag(OPT_WIDTH) { | |||
fmt_opts.width = cmp::max(fmt_opts.goal * 100 / 94, fmt_opts.goal + 3); | |||
fmt_opts.width = cmp::max( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really have anything to do with your change, but are we sure this is right?
I also think the way this is implemented is a bit strange, maybe it would be better in a match expression:
let width_opt = matches.get_one::<String>(OPT_WIDTH);
let goal_opt = matches.get_one::<String>(OPT_GOAL);
match (width_opt, goal_opt) {
(Some(width), Some(goal)) => {...},
...
}
Maybe we can make a good-first-issue for someone to clean this up a bit.
@@ -155,7 +169,10 @@ fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions) | |||
} | |||
}; | |||
if !matches.get_flag(OPT_WIDTH) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to your change, I'm getting a runtime error here because OPT_WIDTH
is not marked as a flag in clap
:
thread 'main' panicked at 'Mismatch between definition and access of `width`. Could not downcast to bool, need to downcast to alloc::string::String
', /home/terts/Programming/uutils/coreutils/src/uu/fmt/src/fmt.rs:171:21
21d30a9
to
09db8d8
Compare
GNU testsuite comparison:
|
I already approved this, but consider this a second approval if the checks are now green (something was up in |
GNU testsuite comparison:
|
This PR implements
Default
forFmtOptions
.I'm not sure whether the defaults for
width
andgoal
, which is derived fromwidth
, are correct. Or whether they differ from the ones used by GNU fmt on purpose. GNU fmt uses awidth
of75
as default, whereas we use79
.